Skip to content

Un-xfail tests which needed fancy indexing; add .view #61

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

ev-br
Copy link
Collaborator

@ev-br ev-br commented Feb 21, 2023

Add a .view method so that we can remove several xfails.

@ev-br ev-br force-pushed the unfail_with_fancy_indexing branch from 9caf080 to 6c85cd3 Compare February 21, 2023 21:47
@ev-br ev-br force-pushed the unfail_with_fancy_indexing branch from 6c85cd3 to c21b892 Compare February 23, 2023 07:38
@ev-br ev-br changed the title Un-xfail tests which needed fancy indexing Un-xfail tests which needed fancy indexing; add .view Feb 23, 2023
@ev-br ev-br requested a review from lezcano February 23, 2023 07:39
@ev-br
Copy link
Collaborator Author

ev-br commented Feb 23, 2023

On CI this fails in __setitem__, in the ndarrays_to_tensors logic, when doing tensor[slice] = list:

self = <test_numeric.TestBoolCmp object at 0x7f5910f1f460>

    def setup_method(self):
        self.f = np.ones(256, dtype=np.float32)
        self.ef = np.ones(self.f.size, dtype=bool)
        self.d = np.ones(128, dtype=np.float64)
        self.ed = np.ones(self.d.size, dtype=bool)
        # generate values for all permutation of 256bit simd vectors
        s = 0
        for i in range(32):
>           self.f[s:s+8] = [i & 2**x for x in range(8)]

torch_np/tests/numpy_tests/core/test_numeric.py:464: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = array_w([1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1.,
        1., 1., 1., 1., 1., 1., 1., 1...., 1., 1., 1.,
        1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1.,
        1., 1., 1., 1.])
index = slice(0, 8, None), value = (0, 0, 0, 0, 0, 0, ...)

    def __setitem__(self, index, value):
        index = _helpers.ndarrays_to_tensors(index)
        index = ndarray._upcast_int_indices(index)
        value = _helpers.ndarrays_to_tensors(value)
>       return self._tensor.__setitem__(index, value)
E       TypeError: can't assign a tuple to a torch.FloatTensor

Any suggestions @honno?

@honno
Copy link
Contributor

honno commented Feb 23, 2023

To me it looks like we need to wrap sequences as tensors as an additional step, as its saying tensors can't handle an input NumPy can

In [1]: t = torch.zeros(256)
[PYFLYBY] import torch

In [2]: t[0:8] = [0. for _ in range(8)]
TypeError: can't assign a list to a torch.FloatTensor

In [3]: x = np.zeros(256)
[PYFLYBY] import numpy as np

In [4]: x[0:8] = [0. for _ in range(8)]

I think this is a regression I caused by replacing asarray(value).get() with _helpers.ndarrays_to_tensors(value), but we shouldn't revert that (we want to handled nested sequences which contain tnp.ndarray elements), so maybe instead asarray() selectively on flat sequence objects...

>>> t[0:8] = torch.as_tensor([0. for _ in range(8)])
# no errors

@@ -155,6 +155,13 @@ def copy(self, order="C"):
tensor = self._tensor.clone()
return ndarray._from_tensor_and_base(tensor, None)

def view(self, dtype):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should to the refactor where methods are implemented in terms of free functions sooner than later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was holding this refactor to minimize the number of conflicts for gh-23. Now that it's in, it's about time indeed.

@honno
Copy link
Contributor

honno commented Feb 23, 2023

so maybe instead asarray() selectively on flat sequence objects...

Something like

from typing import Sequence
...
   def __setitem__(...):
        ...
        if isinstance(value, Sequence) and all(
            isinstance(e, (int, float, complex)) for e in value
        ):
            value = asarray(value).get()
        ...

Doesn't cover nested sequence setting (I'd want to introduce a new thorough test for that—feel free to assign), but this change is easy and supports by-far the most common sequenced-value setting operation.

@lezcano
Copy link
Collaborator

lezcano commented Feb 23, 2023

How does NumPy process these nested sequences into an array? It may be best to simply copy their algorithm.

@ev-br
Copy link
Collaborator Author

ev-br commented Feb 23, 2023

What numpy actually does is well hidden in its C code. Maybe this test is of help: https://github.com/numpy/numpy/blob/main/numpy/core/tests/test_indexing.py#L808

@lezcano
Copy link
Collaborator

lezcano commented Feb 23, 2023

My point is, this logic must be somewhere, in Python, C, or whichever language. We may be able to carve it out and implement it here in Python. That's what we did for plenty of functions in PrimTorch really.
Sometimes this is more difficult than abstracting out the ideas and implementing those, but what we found was that this is often more precise than trying to abstract things out.

@ev-br
Copy link
Collaborator Author

ev-br commented Apr 5, 2023

OK, this PR has run its course. ndarray.view is in main, fancy indexing is TODO, best tackled in one go not piecemeal.

@ev-br ev-br closed this Apr 5, 2023
@ev-br ev-br deleted the unfail_with_fancy_indexing branch April 5, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants